feat: EXC-1735: Move scheduling into the inner round #1757
Conversation
berestovskyy
force-pushed
the
andriy/exc-1735-shedule-inner-round
branch
2 times, most recently
from
0a2a8f3
to
f0ea8a7
Compare
By moving the scheduling strategy into the inner round, we can adjust canister priorities within each round. This allows for greater flexibility and responsiveness to change canister priorities.
berestovskyy
force-pushed
the
andriy/exc-1735-shedule-inner-round
branch
from
f0ea8a7
to
d4cd725
Compare
berestovskyy
changed the title
| @@ -626,9 +626,9 @@ impl SchedulerImpl { | |||
| #[allow(clippy::too_many_arguments, clippy::type_complexity)] | |||
| fn inner_round<'a>( | |||
| &'a self, | |||
| round_log: &ReplicaLogger, | |||
There was a problem hiding this comment.
Personal preference: My approach to argument ordering (and something that, accidentally or not, I see reflected in a lot of places) is:
- at a high level, start with input arguments; then output arguments; and finally incidental stuff, such as logs and metrics;
- within each of these groups (or more likely within the first) go by importance (e.g. first the
stateyou are modifying, then the round, etc.; in this case, beyond thestateit's all a bit subjective).
IOW, I would add the log at the very end (or at the very end, before the metrics).
| self.rate_limiting_of_heap_delta, | ||
| let mut canisters = state.take_canister_states(); | ||
|
|
||
| // Scheduling. |
There was a problem hiding this comment.
High-level comment: Does this change help move us closer to the quick fix? (I.e. does it make it easier to charge all canisters that got a chance at a full round or not?)
Because OTOH, we have both subnets that spend 15-20 ms scheduling; and subnets that do 12 inner loop iterations per round. Luckily no subnet happens to be in both those groups, but even assuming no increase in these numbers, there's nothing stopping a subnet from doing 250 ms worth of scheduling out of a 400 ms round.
There was a problem hiding this comment.
This change shifts the schedule by four canisters per inner round, i.e. potentially 12 times faster. I'm aware of the potential performance impact but have prioritized other optimizations for the upcoming release...
|
|
||
| let metrics = &system_state.canister_metrics; | ||
| // The inner round was skipped once before breaking the round. | ||
| assert_eq!(metrics.skipped_round_due_to_no_messages, 1); |
There was a problem hiding this comment.
Low priority: Could we avoid unnecessarily bumping this counter on every round when we execute all messages?
Or rename it to something like "rounds when we executed all messages"? (Although this can probably be inferred from the number of instructions executed in that round.)
There was a problem hiding this comment.
Sure, we'll have performance optimizations in https://dfinity.atlassian.net/browse/EXC-1617 It's an orthogonal change to this MR.
By moving the scheduling strategy into the inner round, we can adjust canister priorities within each round. This allows for greater flexibility and responsiveness to change canister priorities.